Skip to content

Per-platform binary packages #7395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Apr 14, 2025

Resolves #6133 , Partially #6183

Use symbolic links in dev and replace them with real binaries during the packaging process.

@cometkim cometkim force-pushed the per-platform branch 5 times, most recently from b659400 to 8f1f91c Compare April 15, 2025 00:07
@nojaf
Copy link
Collaborator

nojaf commented Apr 15, 2025

@zth I would assume this will require some LSP server changes as well?

@cometkim cometkim force-pushed the per-platform branch 2 times, most recently from 75f9a9d to 29aa865 Compare April 16, 2025 20:38
@cometkim cometkim force-pushed the per-platform branch 2 times, most recently from 2ae6210 to 887d984 Compare April 16, 2025 20:57
@cometkim cometkim changed the title Per platform Per-platform binary packages Apr 16, 2025
@@ -482,6 +474,11 @@ jobs:
path: |
rescript-${{ env.rescript_version }}.tgz
rescript-std-${{ env.rescript_version }}.tgz
rescript-linux-x64-${{ env.rescript_version }}.tgz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use GitHub's package registry for nightly builds

@cometkim cometkim force-pushed the per-platform branch 2 times, most recently from eeb3455 to 5f53c6d Compare April 16, 2025 22:03
@cometkim cometkim force-pushed the per-platform branch 2 times, most recently from a6f2e53 to 9ac523d Compare April 16, 2025 22:27
@cometkim
Copy link
Member Author

@zth I would assume this will require some LSP server changes as well?

Each platform package will expose binary locations, making it easier to handle.

import { binDir, binPaths } from "@rescript/linux-x64"

@cometkim
Copy link
Member Author

cometkim commented Apr 16, 2025

@cknitt With the current configuration, we have to copy the runtime directory for every platform.

Or we can just extract @rescript/runtime as the default dependency.


Kinda unreliable fix: a4eb634

@cometkim cometkim force-pushed the per-platform branch 2 times, most recently from 34223f4 to e7c1064 Compare April 17, 2025 00:57
@cometkim cometkim marked this pull request as ready for review April 17, 2025 01:55
@cometkim cometkim requested review from cknitt, zth, nojaf and jfrolich April 17, 2025 01:55
// Filename.parent_dir_name // "lib" // "ocaml"
let exe_path = Sys.executable_name in
if Ext_string.contain_substring exe_path ("node_modules" // "@rescript") then
(* node_modules/@rescirpt/{platform}/bins *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(* node_modules/@rescirpt/{platform}/bins *)
(* node_modules/@rescript/{platform}/bins *)

// Filename.parent_dir_name // Filename.parent_dir_name // "rescript"
// "lib" // "ocaml"
else
(* node_modules/rescirpt/{platform} *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(* node_modules/rescirpt/{platform} *)
(* node_modules/rescript/{platform} *)

@cknitt
Copy link
Member

cknitt commented Apr 17, 2025

Or we can just extract @rescript/runtime as the default dependency.

I think that would definitely make sense. 👍

@nojaf
Copy link
Collaborator

nojaf commented Apr 17, 2025

Hi @cometkim, this didn't really work for when I installed the packages using a relative path:

"dependencies": {
    "rescript": "../rescript"
  },
  "devDependencies": {
    "@rescript/darwin-arm64": "../rescript/packages/@rescript/darwin-arm64"
  },

bunx rescript:

>>>> Start compiling
TypeError: Cannot read properties of undefined (reading 'toString')
    at acquireBuild (file:///Users/nojaf/Projects/rescript/cli/common/bsb.js:57:53)
    at delegate (file:///Users/nojaf/Projects/rescript/cli/common/bsb.js:76:12)
    at Module.build (file:///Users/nojaf/Projects/rescript/cli/common/bsb.js:487:5)
    at file:///Users/nojaf/Projects/rescript/cli/rescript.js:134:7
Error: spawn /Users/nojaf/Projects/rescript/packages/@rescript/darwin-arm64/bin/rescript.exe ENOENT
zsh: no such file or directory: /Users/nojaf/Projects/rescript/packages/@rescript/darwin-arm64/bin/rescript.exe

I'm guessing I'm only having this because the symlink thing doesn't work locally.
However, I really use this often, so it would be great if this still worked.

I'm also a fan of having @rescript/runtime. That would be in dependencies, where rescript itself could move to devDependencies again.

Once I downloaded the packages from the GitHub action, it did all work as expected.
I'm able to to run bunx rescript and bunx bsc as I used to.

Tooling broke as I suspected, but we can deal with that. Wouldn't consider that a blocker.

@nojaf
Copy link
Collaborator

nojaf commented Apr 17, 2025

Workaround for tooling would be to set:

    "rescript.settings.platformPath": "/Users/my-user/Projects/my-project/node_modules/@rescript/darwin-arm64/bin"

@cometkim
Copy link
Member Author

this didn't really work for when I installed the packages using a relative path:

@nojaf This is expected behavior of file: protocol of npm/yarn/pnpm/bun. Because the protocol doesn't handle its transitive dependencies.

Make sure your package manager supports other protocols that can handle it properly. If you are using Yarn, you can use portal: protocol instead.

git or tgz protocol might be a workaround.

@nojaf
Copy link
Collaborator

nojaf commented Apr 17, 2025

Well, that is not really a satisfying answer. Can't we just copy the actual binaries to escript/packages/@rescript/darwin-arm64/bin? That way it all works like before right?

@cometkim
Copy link
Member Author

Getting rid of symlinks would be good. I'll try.

But then I'm not sure about the file or link still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: separate packages for prebuilt binaries.
3 participants